Repository navigation
Filter synthesis by the allomorphs used - #548
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #548 +/- ##
==========================================
- Coverage 74.57% 74.57% -0.01%
==========================================
Files 453 453
Lines 38314 38342 +28
Branches 5280 5284 +4
==========================================
+ Hits 28573 28593 +20
- Misses 8576 8583 +7
- Partials 1165 1166 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Approve. The new head traces an allomorph that synthesis drops because analysis did not use it, which closes the last open finding.
Public API, target framework, package, or Findings: F1 addressed; F2 addressed; F3 withdrawn; F4 addressed; F5 addressed; F6 addressed. Reviewed at 57a81e0 |
johnml1135
left a comment
johnml1135
left a comment
There was a problem hiding this comment.
Looks good to me, with two questions and a couple of nits. The filter sits after appliedAllomorphIndices.Add(i) and before the disjunctive break, so an unused allomorph still blocks later ones exactly as on master. I ran #480's conformance suite (140 fixtures) with and without this PR. Nothing changed outcome except the known truncate-morphotactic noise (#506), and the filter fired in eight different rules, so the result means something.
One heads-up for #480: its FieldworksProducibilityLedgerTests lists every FailureReason, and with this PR it fails with Missing (1): "AllomorphNotApplicable". Whichever of #480 and #548 lands second needs to regenerate that ledger row.
docs/review/hermitcrab.md asks for a reproducible measurement when a change claims a speedup. Could you add the before/after synthesis count or wall time for "kanna" in DolganShared? A design question too: synthesis used to try every allomorph, so it could make up for an analysis path that went through the wrong allomorph. With this PR it can't. I found no case in the conformance suite where that matters, but it's worth knowing that analysis now has to find each allomorph's path on its own.
This review was assisted by Claude Opus 5.5.
| && _nonHeadAppIndex == other._nonHeadAppIndex | ||
| && _stratum == other._stratum | ||
| && _rootAllomorph == other._rootAllomorph | ||
| && _allomorphApps.SequenceEqual(other._allomorphApps) |
There was a problem hiding this comment.
Since _allomorphApps is now part of equality, analyses that differ only in which same-form allomorph was unapplied no longer merge, so each one counts against MaxAlternatives. In a probe test (one suffix, three identical +d allomorphs, parse sagd), the smallest MaxAlternatives that parses went from 1 to 3, with merging on or off. The results are the same. Is that intended? FieldWorks defaults the limit to 0, so only projects that set it are affected, but it would be worth a line in the description.
There was a problem hiding this comment.
I hadn't thought of that, but I'd like to keep it as is. One of my goals is that synthesis not do any search. If the morphemes are underspecified, then synthesis of one Word could be exponential in the number of morphemes it has. I want it to be linear in the number of morphemes, if possible (or polynomial at worst).
| _rule, | ||
| i, | ||
| input, | ||
| FailureReason.AllomorphNotApplicable, |
There was a problem hiding this comment.
FieldWorks' FwXmlTraceManager.MorphologicalRuleNotApplied switches on FailureReason and hits default: return; for unknown values. So in Try a Word this allomorph still disappears from the trace. Do you plan a FieldWorks follow-up (a case plus text in FormatHCTrace.xsl), or is hiding it acceptable there?
There was a problem hiding this comment.
I plan to add FailureReason.AllmorphNotApplicable to FieldWorks.
| } | ||
|
|
||
| [Test] | ||
| public void IsAllomorphApplicable() |
There was a problem hiding this comment.
This test builds the analysis Word by hand, so it doesn't check that ParseWord hands the right allomorph to synthesis. It also doesn't cover the cases fixed in review: the disjunctive break, same-form allomorphs differing by MPR feature, and the merged ExpandAlternatives path. Could you add one ParseWord-level test, e.g. -s requiring MPR feature F plus an elsewhere -s, with merging on and off? It would also let SynthesizeWord and LexicalLookup (Morpher.cs:416, :427) stay private. MergeEquivalentAnalyses = true on line 2063 is already the default.
There was a problem hiding this comment.
I'm not sure that I understand what you want. I'm happy for you to upgrade the test after I merge it if you want.
Parsing "kanna" in the DolganShared project takes forever to synthesize because many of the morphemes have lots of unconstrained allomorphs. For instance, the -LA: morpheme has more than 30 unconstrained allomorphs. The synthesizer tries all possible combinations of allomorphs in order to find the combination that produces the input word. One analysis of "kanna" had 5 morphemes that together had ~10,000 allomorph combinations. This was just one of 315 analyses for "kanna".
This pull request solves the problem by recording the allomorphs used during analysis and restricting the synthesizer to only use those allomorphs. This reduces the number of syntheses for "kanna" by four orders of magnitude.
Filtering by allomorphs wasn't as simple as you might imaging because some allomorphs block other free fluctuation allomorphs, so I couldn't just filter allomorphs at the top of the loop.
This change is